Closed Bug 1318006 Opened 3 years ago Closed 3 years ago

Prevent "TypeError: can't access dead object" exceptions on webconsole getCachedMessages

Categories

(DevTools :: Console, defect)

defect
Not set

Tracking

(firefox53 fixed)

RESOLVED FIXED
Firefox 53
Iteration:
53.1 - Nov 28
Tracking Status
firefox53 --- fixed

People

(Reporter: rpl, Assigned: rpl)

References

Details

Attachments

(2 files)

As part of Bug 1211665 (Log messages for background scripts should appear in extension debugger), the originAttributes of the context that has produced the log message is stored in the console message event.

The console message event is also saved in a cache of logged messages, and the cached messages are then retrieved by the webconsole actor when the webconsole panel is loaded.

Unfortunately the originAttributes reference can become a dead object (e.g. because the log message has been produced by a content script's sandbox that has been nuked when the related addon is disabled/uninstalled/reloaded/upgraded) and the webconsole "getCachedMessages RDP request" will fail because of an "accessing dead wrapper" exception raised when the array of cached messages is serialized by 'devtools/shared/transport/transport.js' to be sent to the client.

To prevent the "TypeError: can't access dead object" exception from breaking the webconsole panel loading we should:

- remove the originAttributes property from the cached messages when they are collected by the webconsole actor (in the prepareConsoleMessageForRemote, http://searchfox.org/mozilla-central/source/devtools/server/actors/webconsole.js#1735-1759)

to be able to still filter the console messages using the originAttributes properties even after the context that has originated the console message has been destroyed:

- clone the original originAttributes property when it has been received by the 'dom/console/ConsoleAPIStorage.js' and before it has been stored in the cached messages (in the recordEvent method, http://searchfox.org/mozilla-central/source/dom/console/ConsoleAPIStorage.js#123-139)
Attachment #8811320 - Flags: review?(poirot.alex)
Attachment #8811321 - Flags: review?(amarchesini)
See Also: → 1211665
Comment on attachment 8811320 [details]
Bug 1318006 - Prevent "TypeError: can't access dead object" exception on webconsole getCachedMessages.

https://reviewboard.mozilla.org/r/93478/#review93516

May be we should also consider clearing the console cache when the other scopes like the background page get destroyed.
Here is where we do cleanup when a document is collected:
http://searchfox.org/mozilla-central/source/dom/console/ConsoleAPIStorage.js#66-69
Attachment #8811320 - Flags: review?(poirot.alex) → review+
Comment on attachment 8811321 [details]
Bug 1318006 - Clone originAttributes on cached console message events.

https://reviewboard.mozilla.org/r/93480/#review93520

::: dom/console/ConsoleAPIStorage.js:136
(Diff revision 1)
> +    if (aEvent.originAttributes) {
> +      // Clone the originAttributes object to prevent "accessing dead wrapper"
> +      // exceptions when cached console messages are retrieved by the webconsole
> +      // actor.
> +      aEvent.originAttributes = Cu.cloneInto(aEvent.originAttributes, {});
> +    }

Can you test if we can simply do:

storage.push(Cu.cloneInto(aEvent, {}));
Attachment #8811321 - Flags: review?(amarchesini) → review+
Comment on attachment 8811320 [details]
Bug 1318006 - Prevent "TypeError: can't access dead object" exception on webconsole getCachedMessages.

https://reviewboard.mozilla.org/r/93478/#review93516

I suspect that this is already happening for all the "regular" WebExtension scopes (like a background page): 

when these scopes (e.g. background page, popup pages etc.) are destroyed, the related cached messaged are already cleared by the above code fragment, e.g. because it is going to clear the background messages using the background page "innerWindowID".

On the contrary, this is not going to clear the cached messages generated from a content script scope: 

the content script is emitting its console messages using a console object that is "inherited" by the window hooked by the content script (that is why its console calls are visible in the webconsole related to the tab), and so the cached messages are not going to be cleared when the content script has been nuked, because they are related to the innerWindowID of the window (that is still there).

I'm going to deep into it a bit more, but if the window is reloaded the "TypeError: can't access dead object" does not happen anymore, which seems to confirm what is described above.
Comment on attachment 8811321 [details]
Bug 1318006 - Clone originAttributes on cached console message events.

https://reviewboard.mozilla.org/r/93480/#review93520

> Can you test if we can simply do:
> 
> storage.push(Cu.cloneInto(aEvent, {}));

sure, I tried and we can definitely remove the if statement. 
(I tried locally with both `undefined` and `null` and they can be both cloned without any issue, and I also pushed it to try and it didn't break any of the existent tests).

Updated as suggested in the last version of this patch.
Assignee: nobody → lgreco
Iteration: --- → 53.1 - Nov 28
Status: NEW → ASSIGNED
Pushed by cbook@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/ed8ed6ca5c91
Prevent "TypeError: can't access dead object" exception on webconsole getCachedMessages. r=ochameau
https://hg.mozilla.org/integration/autoland/rev/10ce0ce0261f
Clone originAttributes on cached console message events. r=baku
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/ed8ed6ca5c91
https://hg.mozilla.org/mozilla-central/rev/10ce0ce0261f
Status: ASSIGNED → RESOLVED
Closed: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 53
Product: Firefox → DevTools
You need to log in before you can comment on or make changes to this bug.