Bug 207232 - Web Inspector: ensure that `didClearWindowObjectInWorld` is dispatched for the main world first
Summary: Web Inspector: ensure that `didClearWindowObjectInWorld` is dispatched for th...
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Web Inspector (show other bugs)
Version: WebKit Local Build
Hardware: All All
: P2 Normal
Assignee: Devin Rousso
URL:
Keywords: InRadar
: 207408 (view as bug list)
Depends on: 206110
Blocks:
  Show dependency treegraph
 
Reported: 2020-02-04 14:58 PST by Devin Rousso
Modified: 2020-02-07 16:40 PST (History)
6 users (show)

See Also:


Attachments
Patch (2.84 KB, patch)
2020-02-04 15:01 PST, Devin Rousso
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Devin Rousso 2020-02-04 14:58:10 PST
(In reply to Devin Rousso from comment https://webkit.org/b/206110#c16)
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=388830&action=review
> 
> >> Source/WebCore/inspector/agents/page/PageRuntimeAgent.cpp:178
> >> +    auto injectedScript = injectedScriptManager().injectedScriptFor(globalObject);
> > 
> > I believe there is a subtle issue here - when didClearWindowObjectInWorld is called for the User world before it is called for the Normal world, subsequent call to clear the normal world will erase this injected script from the InjectedScriptManager through m_injectedScriptManager->discardInjectedScripts().
> > 
> > This can either be patched via the following change to the dispatchDidClearWindowObjectsInAllWorlds (that is somewhat hacky), or via going back to instrumenting only normal worlds and iterating over the remaining worlds within agents explicitly.
> > 
> > void FrameLoader::dispatchDidClearWindowObjectsInAllWorlds()
> > {
> >     Vector<Ref<DOMWrapperWorld>> worlds;
> >     ScriptController::getAllWorlds(worlds);
> >     // It is essential that the normal world is cleared first.
> >     // Various subsystem (InjectedScriptManager) will reset state upon normal
> >     // world initialization.
> >     Vector<DOMWrapperWorld*> nonNormalWorlds;
> >     for (auto& world : worlds) {
> >         if (world->type() == DOMWrapperWorld::Type::Normal)
> >             dispatchDidClearWindowObjectInWorld(world);
> >         else
> >             nonNormalWorlds.append(&world.get());
> >     }
> >     for (auto* world : nonNormalWorlds)
> >         dispatchDidClearWindowObjectInWorld(*world);
> > }
> 
> Interesting!  That is super subtle :P
> 
> I think we'd ideally want to dispatch for the `mainThreadNormalWorld()`
> first (as that's really the "signpost" for resetting all the injected
> scripts and debugger state), then all other normal worlds, and then all
> other worlds.  IIUC, there should only ever be one normal world though (and
> the current code supports that), so I think we can just do as you suggest.
> 
> @Pavel Feldman, do you want to fix this, or should I?
Comment 1 Devin Rousso 2020-02-04 15:01:21 PST
Created attachment 389716 [details]
Patch
Comment 2 WebKit Commit Bot 2020-02-05 18:13:52 PST
Comment on attachment 389716 [details]
Patch

Clearing flags on attachment: 389716

Committed r255885: <https://trac.webkit.org/changeset/255885>
Comment 3 WebKit Commit Bot 2020-02-05 18:13:53 PST
All reviewed patches have been landed.  Closing bug.
Comment 4 Radar WebKit Bug Importer 2020-02-05 18:14:17 PST
<rdar://problem/59210284>
Comment 5 Pavel Feldman 2020-02-07 16:40:39 PST
*** Bug 207408 has been marked as a duplicate of this bug. ***